-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create physionet-build Github CI action #1320
Conversation
[WIP] This github action will replace the shippable.yml for build-test.
Remove excess tabs to prevent errors.
Exit with code 127 EDIT
|
You should be able to fix the postgres error by adding postgres as a service:
The manual installation can then be removed:
|
On Wed, 14 Apr 2021 20:45:33 -0700, Tom Pollard ***@***.***> wrote:
You should be able to fix the postgres error by adding postgres as a
service:
I think that is not a good idea. (a), we want to test against the
Debian stable version of postgres, not whatever version is supplied by
github. And (b), this script should be self-contained, not dependent
on the provider. We need to be able to spin up a virtual machine of
our own and run the same exact commands, and it should give
exactly the same results as the automated test.
(The problem at c182dd5 is that you're
not invoking the psql commands as the "postgres" user. You need to
invoke those using sudo -u postgres.)
It seems it's impossible to view the logs without being logged in to
Github? That's a serious problem.
|
Yep, fair point! |
What other options do we have? GHA seems a little restrictive but I think
it can get close enough without being exactly the same as how it previously
was. If this is going to be shot down then we need an alternative.
…On Thu, Apr 15, 2021 at 12:47 Tom Pollard ***@***.***> wrote:
I think that is not a good idea. (a), we want to test against the
Debian stable version of postgres, not whatever version is supplied by
github.
Yep, fair point!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1320 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJNM7BSDJ4ZGPJ6WPAGOY43TI4KDPANCNFSM4256BVBA>
.
|
I think we go back to the original approach, which is to install and start a postgres service on the server. |
I think we go back to the original approach, which is to install and
start a postgres service on the server.
Yeah, that ought to work if we get the permissions correct.
Brian, you said in ad7dead that
Docker runs as root; I'm not sure if that's true or not, but either
way, there are some commands (like installing lightwave) that require
root privileges, while other commands (e.g. manage.py test)
specifically *should* be run as a non-root user so that we know the
permission checks work correctly.
|
In this case, we probably need to add a step at the beginning of the process to create some kind of build user. Seemed strange to be running everything as root anyway. |
I'll look into setting up a separate user from root. @bemoody , do we need to store a file for the logs? |
I don't really know Docker, I've only used it occasionally, but I'd be
a little surprised if Docker (or Github) didn't set up an unprivileged
account within the container by default.
I'm not going to worry too much about the logs for now. I think we
would be better off in a lot of ways running these tests on our own
servers (e.g. using jenkins or buildbot) but whether this is more or
less work, overall, than dealing with the annoyances of Github, I'm not
sure.
|
@bemoody I confirmed with "USER |
I see. I am not sure what they mean exactly about accessing
GITHUB_WORKSPACE.
Here somebody suggests specifying a UID in container.options:
https://github.community/t/how-to-run-action-as-a-non-root-user/17572
(why 1001, though, I have no idea.)
|
Yeah, I'd also seen that. I wasn't able to figure out how to create a non-root user before issuing the This gives a bit of information about GITHUB_WORKSPACE: GITHUB_WORKSPACE | The GitHub workspace directory path. The workspace directory is a copy of your repository if your workflow uses the actions/checkout action. If you don't use the actions/checkout action, the directory will be empty. For example, /home/runner/work/my-repo-name/my-repo-name. I can look into other options for checking out the repository without using the |
I can run a given command ( Also, I realized that the choice of |
I can run a given command (`whoami` in this case) as a non-root user
with: `sudo -u physionet_user whoami`
Maybe we could do something like this?
- name: create an unprivileged user account
run: |
/sbin/useradd pntestuser
chown pntestuser.pntestuser -R .
...
- name: setup and test physionet, check amount of code tested
shell: sudo -u pntestuser bash -e {0}
run: |
python physionet-django/manage.py makemigrations --dry-run --no-input --check
python physionet-django/manage.py resetdb
python physionet-django/manage.py loaddemo
python physionet-django/manage.py test --verbosity=3 --keepdb
coverage run --source='.' physionet-django/manage.py test --keepdb
coverage report -m
Though sudo might also monkey with environment variables like PATH...
|
Thanks @bemoody , your suggestion does work for setting up a pntestuser and switching to that user for a given I'm still trying to work out if it's possible to clone physionet-build to a location that is not under $GITHUB_WORKSPACE since we can only access that workspace as root (which I confirmed). Instead of using the github action
running
|
@bemoody , from what I can tell Github actions doesn't allow access to cloned repositories (in the GITHUB_WORKSPACE) while running as a non-root user. This post explains some of the restrictions around not being able to clone outside of the GITHUB_WORKSPACE: "Today the path input is the only way to change the directory where the code is cloned. Also there is a restriction that it must be under GITHUB_WORKSPACE." As mentioned previously, you can't access the GITHUB_WORKSPACE unless you are root. A workaround would be to use a self-hosted runner but I don't think we want to do that. I could ask in a forum to check to see if there is a way around this that I haven't been able to find so far. |
I think this will probably work but we need to think seriously about
how to improve the testing workflow.
It looks like neither these nor the shippable tests are run when the
dev branch is updated, so the final result isn't being tested until
*after* it's merged. Of course we still can test things after merging
into dev and before deploying, but I feel like it would be better for
the pull request to be blocked until it's been fully tested.
However, unless you see any easy fixes, I think this meta discussion
should happen separately.
|
As this is currently just supplementing existing tests, I'd be good to merge now if everyone else is okay with that? We can then add new issues for (1) fixing the user issue and (2) triggering rebuilds on pull requests when the target is updated. |
Can you explain more of what you mean by that? This:
should be triggering tests whenever a commit is pushed to Are you saying we require all checks to pass before it can be merged? I think this is just a protected branch issue and not GHA issue. |
Can you explain more of what you mean by that? This:
```
on:
push:
branches:
- dev
pull_request:
branches:
- dev
```
should be triggering tests whenever a commit is pushed to `dev` or a
pull request is targeting `dev` (new commits to the pull request also
trigger tests).
If I understand correctly, it runs in two cases:
- When a commit is pushed to dev, it runs the tests on the current
state of dev. This has no effect on pull requests and doesn't take
pull requests into account.
- When a pull request is created, or a commit is pushed to a branch
linked to a pull request, it tests the result of merging that
commit with the *current* state of dev.
So when there are two or more pull requests at once, and you merge one
of them and push it to dev, it doesn't re-test the other branches
against the new state of dev.
For example, here, it appears to have run tests for
- briangow-patch-3~1 (454b121) with dev~1 (e0f883c)
(merged as 5ee9aa2, tested by run 755909966)
- briangow-patch-3 (77f1768) with dev (2639176)
(merged as 314098a, tested by run 768130278)
but after 2639176 was pushed to dev it never tested the result of
merging 454b121 with 2639176.
|
Improvements are welcome but let's discuss them elsewhere. :)
|
[WIP] This github action will replace the shippable.yml for build-test.
This action is my initial attempt to replicate the process that has been used in the shippable.yml with Github actions. Please do not merge it yet.
I've added descriptions for what the code is doing in the
-name:
fields. If you could review these and suggest corrections it would be appreciated.Regarding the lines of code starting with
coverage
. This code is designed to report on how much of the code was actually executed: https://coverage.readthedocs.io/en/coverage-5.5/ . Do we want to keep doing these coverage checks? If so, is it sufficient to report the result in the log instead of saving it to a file as was done in shippable.yml?